Skip to content

Conversation

@nerdrew
Copy link
Contributor

@nerdrew nerdrew commented Dec 19, 2025

No description provided.

@nerdrew nerdrew force-pushed the wrongpass-close branch 3 times, most recently from 4c745b1 to f20612c Compare December 19, 2025 22:18
@byroot byroot marked this pull request as ready for review December 19, 2025 22:27
@byroot byroot marked this pull request as draft December 19, 2025 22:27
connect_error.set_backtrace(error.backtrace)
raise connect_error
rescue CommandError => error
@raw_connection&.close
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I like about closing the connection is that the WRONGPASS and many other errors could be the result of a recent failover or some other issues of the sort, and to recover from it, you may need to redo the DNS resolution.

Hence not only I think closing the connection is better, but I think we should also do it in case of FailoverError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should FailoverError be handled differently from CannotConnectError then? I.e. should CannotConnectError also have @raw_connection&.close?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all errors during connect should drop the connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I've updated this PR.

@nerdrew nerdrew marked this pull request as ready for review December 22, 2025 18:03
@byroot byroot merged commit 0c5cffa into redis-rb:master Dec 22, 2025
@nerdrew nerdrew deleted the wrongpass-close branch December 22, 2025 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants